- 
                Notifications
    You must be signed in to change notification settings 
- Fork 489
          [openrewrite] add SanityCheck featuring CommonStaticAnalysis
          #2651
        
          New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
74a991e    to
    c79f4ca      
    Compare
  
            
          
                ...xtra/src/groovy/java/com/diffplug/spotless/extra/glue/groovy/GrEclipseFormatterStepImpl.java
          
            Show resolved
            Hide resolved
        
      c79f4ca    to
    1171328      
    Compare
  
    com.diffplug.spotless.SanityCheckcom.diffplug.spotless.SanityCheck featuring CommonStaticAnalysis
      com.diffplug.spotless.SanityCheck featuring CommonStaticAnalysisSanityCheck featuring CommonStaticAnalysis
      1171328    to
    30ce140      
    Compare
  
    …tor and dereferenced in com.diffplug.spotless.sql.dbeaver.SQLTokensParser.nextToken() [openrewrite] add `SanityCheck` featuring `CommonStaticAnalysis`
…mmonStaticAnalysis # Conflicts: # lib/src/main/java/com/diffplug/spotless/sql/dbeaver/SQLTokensParser.java
| import org.springframework.boot.autoconfigure.SpringBootApplication; | ||
|  | ||
| @ | ||
|  | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
undo. as test resource.
this passing CI is an issue, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CI seems happy, but we should not run this openrewrite tool on src/main/resources I don't think
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hm yes can exclude this later on as well. Some of the recipes like line endings or headers apply to this as well.
Imho we drive best with convention over configuration principle, adjusting only on breaking changes like this.
Dedicated this file to exclusion.
| I should try running this or the main branch sooner or later and see how fast it runs compared to before OpenRewrite's introduction. In my experience, the OpenRewrite Gradle plugin is orders of magnitude slower than Spotless on a small personal project and it doesn't benefit from incremental processing. But maybe the cost is amortised more on a larger project like Spotless itself. If so, I'd be more than happy to see OpenRewrite's continued use here. Worst case, I could follow up on my earlier effort to make OpenRewrite configuration-cache-compatible, which I believe would make the plugin faster. | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
okay, whatever you do, do not force-push! this was huge and took forever to review, but it looks good.
I see two issues:
- we need to get testlib/src/main/resourcesout of it
- there's one change in sql/DBeaver which is nonsensical
- ignore that file
- or i suppose it's okay to delete that comment
 
        
          
                lib/src/main/java/com/diffplug/spotless/sql/dbeaver/SQLTokensParser.java
          
            Show resolved
            Hide resolved
        
      | import org.springframework.boot.autoconfigure.SpringBootApplication; | ||
|  | ||
| @ | ||
|  | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CI seems happy, but we should not run this openrewrite tool on src/main/resources I don't think
| how to apply changes like this to maven ? @slachiewicz Imho we need some tool to achieve this on large scale. | 
| not sure why exclusing 9c0fab9 caused so many changes, seems like adding just one exclude should cascade through huge changes.  | 
…tor and dereferenced in com.diffplug.spotless.sql.dbeaver.SQLTokensParser.nextToken() [openrewrite] add `SanityCheck` featuring `CommonStaticAnalysis`
…mmonStaticAnalysis # Conflicts: # gradle/rewrite.gradle
| rebase. seems ready now. Thanks for the invest. | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
| Loving the fixes to the various classes to make them  It's on my TODO list to follow up on the performance side of OpenRewrite. | 
No description provided.